-
Notifications
You must be signed in to change notification settings - Fork 439
fix(model): allow backwards compat as per MCP spec #357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(model): allow backwards compat as per MCP spec #357
Conversation
|
@alexhancock PTAL again - not sure about the comment lint check though |
b83bc36 to
7f33cdf
Compare
| assert_eq!(result.is_error, Some(true)); | ||
| } | ||
|
|
||
| #[tokio::test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe keep the test and check that it it doesn't error at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub doesn't let me comment outside the diff, but I think the CallToolResult struct docstring needs adjustment. The last line needs to be removed:
///
/// Note: `content` and `structured_content` are mutually exclusive - exactly one must be provided.
|
@crepererum I addressed the feedback in #359 - not pushing to this one since it's @michaelneale's fork and I won't have access |
|
Merged #359 which has these changes! |
This is a very small fix for issue: #355
MCP servers (especially in python) will likely fail with 0.4.0 with an error like "Error: Unexpected response type"
this is due to the validation
content and structured_content are mutually exclusive, but this is a bit strict.How Has This Been Tested?
Tested with goose and a python based MCP server - it worked with < 0.4.0 and failed with 0.4.0.
I then tested this patch with the goose agent and exactly the same server it works.
Breaking Changes
None
Types of changes
Checklist